Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adding support for intersection of arrays and tuples #1237

Merged
merged 4 commits into from
May 6, 2022

Conversation

filipomar
Copy link
Contributor

@filipomar filipomar commented May 1, 2022

Adding support for intersection of arrays and tupples such as:

type NonEmptyArray<T> = [T, ...T[]] & T[];

Types originally from here.

Version

Published prerelease version: v1.1.0-next.3

Changelog

🎉 This release contains work from new contributors! 🎉

Thanks for all your work!

❤️ null@filipomar

❤️ Remi Cattiau (@loopingz)

❤️ Hadrien Milano (@hmil)

🚀 Enhancement

🐛 Bug Fix

🔩 Dependency Updates

Authors: 8

return this.childTypeFormatter.getDefinition(types[0]);
}

const requirements: Definition[] = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dependencies may be a better word.

@filipomar filipomar requested a review from domoritz May 2, 2022 08:24

types.forEach((t) => {
if (t instanceof ArrayType || t instanceof TupleType) {
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use // comments in code

const dependencies: Definition[] = [];
const nonArrayLikeTypes: BaseType[] = [];

types.forEach((t) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use for ... instead of forEach

});

if (nonArrayLikeTypes.length) {
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

if (t instanceof ArrayType || t instanceof TupleType) {
/**
* Arrays are not easily mergeable
* So it's just easier to append their defs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add these comments higher up and focus more on the why than the what.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I quite understand what you mean by the "why vs what". I will be moving the comments higher up in the in chain for now though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments should explain why we are doing something and now what is happening in the code.

@filipomar filipomar force-pushed the array-and-tuple-intersection branch from f6112e9 to 2c2e655 Compare May 3, 2022 16:19
@domoritz domoritz merged commit 243ee05 into vega:next May 6, 2022
@filipomar filipomar deleted the array-and-tuple-intersection branch May 25, 2022 15:04
@github-actions
Copy link

🚀 PR was released in v1.1.0 🚀

@github-actions github-actions bot added released This issue/pull request has been released. and removed prerelease labels Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants